fix(pds-core): identify generated-handle accounts by email - #244

packages/pds-core/src/chooser-enrichment.ts (2)

482-529: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Hoist the per-tick helper declarations out of enrich().

accountListAnchor, emptyAccountTitle, and enrichAccountListRow are declared inside enrich(). enrich() runs from the MutationObserver callback on Line 673, which observes document.documentElement with subtree: true. enrich() itself mutates the DOM: it appends spans, sets style properties, and sets attributes. Each of those mutations schedules another tick.

The dataset guards make repeat ticks cheap per node, but each tick still allocates three closures and performs a full TreeWalker pass over #root, plus the querySelectorAll calls in enrichAccountSelector and hideSignup. This PR increases the per-tick cost with the added consent walker and selector walker.

Move the three helpers to the same scope as the other top-level script functions. Consider also coalescing observer ticks with requestAnimationFrame so a burst of self-inflicted mutations produces one pass.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/pds-core/src/chooser-enrichment.ts` around lines 482 - 529, Move
accountListAnchor, emptyAccountTitle, and enrichAccountListRow out of enrich()
into the same top-level scope as the other script functions, preserving their
existing behavior and dependencies. Keep enrich() focused on orchestration so
observer-triggered ticks do not recreate these closures; optionally coalesce
MutationObserver-triggered enrich passes with requestAnimationFrame if supported
by the existing flow.

---

294-300: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Make consent identity enrichment locale-independent.

@atproto/oauth-provider-ui@0.4.3 falls back to navigator.languages when uiLocales is absent. French and Japanese consent copy therefore bypasses hasApprovedConsentIdentityContext. Match a structurally identified account element while retaining the exact known handle, @handle, or DID check.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/pds-core/src/chooser-enrichment.ts` around lines 294 - 300, Update
hasApprovedConsentIdentityContext to identify the consent account element
structurally rather than matching only English phrases in context.before, so
localized French and Japanese copy is supported. Preserve the existing exact
checks for a known handle, `@handle`, or DID, and keep the context.after ===
'account' requirement.

packages/pds-core/src/__tests__/preview-consent.test.ts (1)

83-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Isolate EPDS_DEFAULT_HANDLE_MODE in this test.

resolveHandleMode reads this variable at render time. Stub it to an empty value before rendering and call vi.unstubAllEnvs() in the existing afterEach; vi.restoreAllMocks() does not restore environment stubs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/pds-core/src/__tests__/preview-consent.test.ts` around lines 83 -
103, Update the test around createPreviewConsentHandler to stub
EPDS_DEFAULT_HANDLE_MODE to an empty value before rendering, ensuring
resolveHandleMode reads the isolated environment. Extend the existing afterEach
cleanup to call vi.unstubAllEnvs() alongside vi.restoreAllMocks().</codeેન

packages/pds-core/src/__tests__/chooser-enrichment.test.ts (2)

1939-1955: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Make the redaction guard see error messages.

JSON.stringify serializes an Error instance to {}, because message and stack are non-enumerable. The err field in the logged payload is therefore invisible to the not.toContain(requestUri) guard. The test cannot catch a regression where the request URI reaches the log through an error message.

Normalize errors before stringifying so the guard covers the whole payload.

🔒️ Proposed hardening

     expect(debug).toHaveBeenCalledWith(
       expect.objectContaining({ hasRequestUri: true }),
       'chooser-enrichment: failed to resolve handle mode from OAuth request context',
     )
-    expect(JSON.stringify(debug.mock.calls)).not.toContain(requestUri)
+    const serialized = JSON.stringify(debug.mock.calls, (_key, value) =>
+      value instanceof Error ? `${value.name}: ${value.message}` : value,
+    )
+    expect(serialized).not.toContain(requestUri)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/pds-core/src/__tests__/chooser-enrichment.test.ts` around lines 1939
- 1955, Update the redaction assertion in the “logs request_uri presence but
never its value” test to normalize Error values, including their message and
stack, before stringifying debug.mock.calls. Keep the existing request URI
absence check, but ensure it inspects the complete logged payload so a URI
embedded in an error message is detected.

---

1842-1855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that resolveClientMetadata is not called after client-id resolution rejects.

The test supplies resolveClientMetadata: vi.fn() but never asserts on it. vi.fn() returns undefined, so a regression that calls resolveClientMetadata(undefined) after the rejection still yields the fallback meta tag and the test still passes. Assert the skip explicitly.

♻️ Proposed assertion

   it('degrades silently when request_uri client-id resolution rejects', async () => {
+    const resolveClientMetadata = vi.fn()
+
     const written = await captureWrittenHtml(
       {
-        resolveClientMetadata: vi.fn(),
+        resolveClientMetadata,
         resolveClientIdFromRequestUri: () =>
           Promise.reject(new Error('request expired')),
       },
       { request_uri: 'urn:ietf:params:oauth:request_uri:req-123' },
     )
 
+    expect(resolveClientMetadata).not.toHaveBeenCalled()
     expect(written).toContain(
       '<meta name="epds-handle-mode" content="picker-with-random">',
     )
   })

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/pds-core/src/__tests__/chooser-enrichment.test.ts` around lines 1842
- 1855, Update the test case around captureWrittenHtml to assert that the
supplied resolveClientMetadata mock was not called when
resolveClientIdFromRequestUri rejects. Keep the existing fallback meta-tag
assertion and use the existing resolveClientMetadata mock for the explicit
call-count check.

e2e/step-definitions/consent.steps.ts (1)

69-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make openIdentityTooltip idempotent.

The tooltip control toggles on click. Line 82 asserts aria-expanded is 'false' before line 90 clicks it. Two Then steps call this helper: "the consent identity tooltip exposes the public AT Protocol handle" and "the consent identity tooltip exposes the account email". No scenario chains both steps today, so the helper works. If a future scenario chains them, the second call finds the tooltip already open and the line 82 precondition fails with a misleading message.

Open the tooltip only when it is closed.

♻️ Proposed change

   await expect(tooltipControl).toHaveAttribute('type', 'button')
-  await expect(tooltipControl).toHaveAttribute('aria-expanded', 'false')
   const describedBy = await tooltipControl.getAttribute('aria-describedby')
   expect(describedBy?.trim()).toBeTruthy()
   if (!describedBy?.trim()) {
     throw new Error('Expected aria-describedby to reference a tooltip')
   }
   const [tooltipId] = describedBy.trim().split(/\s+/)
 
-  await tooltipControl.click()
+  // Idempotent: a scenario may chain several tooltip assertions, and the
+  // control toggles rather than latches open.
+  if ((await tooltipControl.getAttribute('aria-expanded')) !== 'true') {
+    await tooltipControl.click()
+  }
   await expect(tooltipControl).toHaveAttribute('aria-expanded', 'true')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/step-definitions/consent.steps.ts` around lines 69 - 97, Update
openIdentityTooltip so it reads aria-expanded and clicks the tooltip control
only when its current value is not already 'true'. Preserve the existing
closed-state validation for the initial call, while allowing subsequent calls to
reuse the already-open tooltip and continue validating and returning it.

e2e/step-definitions/session-reuse-bugs.steps.ts (1)

440-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert that the hidden-handle description reaches the accessibility tree.

The step proves the description element exists, carries .epds-hidden-handle-description, and holds the expected text. It does not prove that assistive technology can announce it. If the enrichment styled the description with display: none or visibility: hidden, screen readers would announce nothing and this step would still pass. That defeats the accessibility guarantee this PR introduces.

Capture the computed style of the resolved description and assert it is not removed from the accessibility tree.

♻️ Proposed additions

 type HiddenHandleDescriptionRow = {
   describedBy: string | null
   descriptions: {
     id: string
     isHiddenHandleDescription: boolean
+    isAccessible: boolean
     text: string
   }[]

             const descriptions = descriptionIds.map((id) => {
               const describedElement = document.getElementById(id)
+              const style = describedElement
+                ? globalThis.getComputedStyle(describedElement)
+                : null
               return {
                 id,
                 isHiddenHandleDescription:
                   describedElement?.classList.contains(
                     'epds-hidden-handle-description',
                   ) ?? false,
+                // display:none / visibility:hidden remove the node from the
+                // accessibility tree, so aria-describedby would announce nothing.
+                isAccessible:
+                  style !== null &&
+                  style.display !== 'none' &&
+                  style.visibility !== 'hidden',
                 text: describedElement?.textContent?.trim() ?? '',
               }
             })

Then assert it in the loop:

       expect(
         description,
         `Row ${row.rowIndex}: expected aria-describedby to reference an .epds-hidden-handle-description element`,
       ).toBeDefined()
+
+      expect(
+        description?.isAccessible,
+        `Row ${row.rowIndex}: hidden-handle description must stay in the accessibility tree (not display:none / visibility:hidden)`,
+      ).toBe(true)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@e2e/step-definitions/session-reuse-bugs.steps.ts` around lines 440 - 478,
Update the evaluateAll callback and its HiddenHandleDescriptionRow result to
capture each resolved description element’s computed display and visibility
values. In the assertion loop consuming these rows, require the hidden-handle
description to have display other than none and visibility other than hidden,
while preserving the existing class and text checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes